Enable `warn_unreachable` for `mypy` self-check by sobolevn · Pull Request #18523 · python/mypy

sobolevn · 2025-01-24T09:21:08Z

This check prooved to be useful, since it find a lot of dead / incorrect code.

Closes #18079

for more information, see https://pre-commit.ci

hauntsaninja

Awesome. I think it could worth replacing some of these with asserts, just in case there's something mypy has got wrong

sobolevn · 2025-01-24T09:44:42Z

I think it could worth replacing some of these with asserts

Do you have some specific items in mind? I can surely add asserts where needed.

…ue-18079

A5rocks · 2025-01-24T19:37:21Z

I think it could worth replacing some of these with asserts

Do you have some specific items in mind? I can surely add asserts where needed.

I actually just did this too! I ended up with quite a few asserts so maybe that can be a second opinion. master...A5rocks:mypy:experiments/warn_unreachable (I also deferred on mypy_extensions.trait subclassing instead of updating the isinstance)

Though that branch also passes mypy's test suite, so maybe that's a sign that actually those asserts aren't necessary at all and mypy was correct.

A5rocks

I'm generally unsure about the isinstance(x, FuncBase) changes where x: Node (and similar mypy_extensions.trait workarounds) because it seems like that just uses tuples to work around reachability? I haven't tried some testcases locally so I'm not entirely sure though.

A5rocks · 2025-01-24T19:40:56Z

mypy/util.py

-        assert sys.platform == "win32"
-        if sys.platform == "win32":
+        assert sys.platform == "win32", "Running not on Windows"
+        if sys.platform == "win32":  # needed to find win specific sys apis


You can un-indent this.

Nope, sadly I can't :(
Looks like that mypy does not narrow sys.platform after assert (this might be a bug).

But, I added assert False in the end instead, it is more readable now.

Nope, sadly I can't :( Looks like that mypy does not narrow sys.platform after assert (this might be a bug).

Huh I was able to. But I guess that might just be because I was testing on Windows.

Yes, you need to set platform = win32 or platform = linux manually in .ini file for that.

mypy/util.py

mypy_self_check.ini

mypy/test/testmerge.py

mypy/test/data.py

mypy/server/astdiff.py

sobolevn · 2025-01-24T21:12:02Z

I'm generally unsure about the isinstance(x, FuncBase) changes where x: Node (and similar mypy_extensions.trait workarounds) because it seems like that just uses tuples to work around reachability?

Sorry, I don't understand your question. Can you please provide an example?

mypy/constraints.py

mypy/nodes.py

mypy/server/mergecheck.py

A5rocks · 2025-01-25T06:19:51Z

I'm generally unsure about the isinstance(x, FuncBase) changes where x: Node (and similar mypy_extensions.trait workarounds) because it seems like that just uses tuples to work around reachability?

Sorry, I don't understand your question. Can you please provide an example?

I was confused, actually. I've now read the docstring of FuncBase and I get why you switched out the isinstance checks... Except it refers to an issue that's closed! You're just doing #13607 (comment) (3), which will mean we have to revert a bunch of lines whenever we actually fix the issue (1).

IMO we should fix the issue at hand but continuing with (3) is OK as long as you update the comment around SYMBOL_FUNCBASE_TYPES (and maybe put a TODO or something there?).

sobolevn · 2025-01-26T08:03:56Z

I will leave this open for a couple of days to give an opportunity for second rounds of reviewes, and merge on Wen or so :)

Thanks, @A5rocks!

ammaraskar · 2025-01-26T20:56:57Z

mypy/stubgen.py



 def normalize_path_separators(path: str) -> str:
-    if sys.platform == "win32":


If #18539 or a similar fix is merged, then this change should no longer be necessary I believe.

sobolevn · 2025-02-13T15:50:47Z

Going to merge #18523 later tomorrow, if no one objects

mypy/nodes.py

github-actions · 2025-02-13T22:48:41Z

Diff from mypy_primer, showing the effect of this PR on open source code:

freqtrade (https://github.com/freqtrade/freqtrade): 1.95x slower (129.1s -> 252.2s in a single noisy sample)

This check prooved to be useful, since it find a lot of dead / incorrect code. Closes python#18079 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Enable warn_unreachable for mypy self-check

f45652d

sobolevn requested review from JukkaL, hauntsaninja and ilevkivskyi January 24, 2025 09:21

[pre-commit.ci] auto fixes from pre-commit.com hooks

867958e

for more information, see https://pre-commit.ci

This comment has been minimized.

Sign in to view

hauntsaninja approved these changes Jan 24, 2025

View reviewed changes

sobolevn added 3 commits January 24, 2025 12:44

Fix CI

792ba28

Merge branch 'issue-18079' of https://github.com/python/mypy into iss…

4607277

…ue-18079

Fix CI

b65e27a

This comment has been minimized.

Sign in to view

Fix CI

8a73b5d

This comment has been minimized.

Sign in to view

A5rocks reviewed Jan 24, 2025

View reviewed changes

Address review

bbb830f

ilevkivskyi reviewed Jan 24, 2025

View reviewed changes

mypy/constraints.py Show resolved Hide resolved

ilevkivskyi reviewed Jan 24, 2025

View reviewed changes

mypy/nodes.py Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ilevkivskyi reviewed Jan 24, 2025

View reviewed changes

mypy/server/mergecheck.py Show resolved Hide resolved

Address reviews

768b54c

This comment has been minimized.

Sign in to view

A5rocks approved these changes Jan 26, 2025

View reviewed changes

ammaraskar reviewed Jan 26, 2025

View reviewed changes

sobolevn added 2 commits February 13, 2025 18:02

Merge branch 'master' into issue-18079

547a6d7

Update nodes.py

4859d91

This comment has been minimized.

Sign in to view

A5rocks reviewed Feb 13, 2025

View reviewed changes

mypy/nodes.py Outdated Show resolved Hide resolved

sobolevn added 2 commits February 14, 2025 01:15

Merge branch 'master' into issue-18079

bf56d55

Address review

c5e56cb

sobolevn merged commit 555bfae into master Feb 14, 2025
18 checks passed

sobolevn deleted the issue-18079 branch February 14, 2025 06:13



		def normalize_path_separators(path: str) -> str:
		if sys.platform == "win32":

Uh oh!

Conversation

sobolevn commented Jan 24, 2025

Uh oh!

This comment has been minimized.

hauntsaninja left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Jan 24, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

A5rocks commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

A5rocks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A5rocks Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

sobolevn Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

sobolevn Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

A5rocks Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

sobolevn Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sobolevn commented Jan 24, 2025

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

A5rocks commented Jan 25, 2025

Uh oh!

This comment has been minimized.

sobolevn commented Jan 26, 2025

Uh oh!

ammaraskar Jan 26, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

sobolevn commented Feb 13, 2025

Uh oh!

Uh oh!

github-actions bot commented Feb 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hauntsaninja left a comment •

edited

Loading

A5rocks commented Jan 24, 2025 •

edited

Loading

A5rocks left a comment •

edited

Loading